-
Notifications
You must be signed in to change notification settings - Fork 50
Include Exception Properties at FailureDetails #474
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Conversation
} | ||
} | ||
|
||
static P.TaskFailureDetails? GetFailureDetails(FailureDetails? failureDetails) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
remove this because we will call the one at ptobufutils instead
@@ -0,0 +1,17 @@ | |||
// Copyright (c) Microsoft Corporation. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question: is this the right place to put this file? Customer need to refer this interface so they need to use this package. I guess I should put it here but I am not 100% sure.
exception.StackTrace, | ||
FromExceptionRecursive(exception.InnerException)); | ||
FromExceptionRecursive(exception.InnerException), | ||
null);// might need to udpate this later |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Question for the public static TaskFailureDetails FromException(Exception exception)
I checked all the reference, and it seems all the cases we called this is when the exception is a DTCore.OrchestrationExcep, which means it should already have properties included. Thus currently we don't need to extract properties here.
But I am not sure if there might be a case when the exception is not from dt.core and this return new ... is reached. If so, then we need to extract properties in this repo, which means I need to update OrchestrationContextWrapper
class etc. But I didn't find a case like this from current code path. Let me know if there is any other thought on this.
? new(new(p.Name), p.OrchestrationInstance.InstanceId) | ||
: null; | ||
|
||
DurableTaskShimFactory factory = services is null |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I only updated line 211 to 226.. the whole file changed because of the VS outlining thing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
A few initial comments. Haven't reviewed everything yet.
@@ -1,2 +1,2 @@ | |||
# The following files were downloaded from branch main at 2025-09-17 01:45:58 UTC | |||
https://raw.githubusercontent.com/microsoft/durabletask-protobuf/f5745e0d83f608d77871c1894d9260ceaae08967/protos/orchestrator_service.proto | |||
# The following files were downloaded from branch nytian/failure-details at 2025-10-01 21:51:24 UTC |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reminder to update this to main
once the protobuf changes in durabletask-protobuf are merged.
This PR supports register custom provider that help include exception properties at
TaskFailureDetails
. Supports include running with both DurableTaskWorker and Durable Functions (.NET Isolated).Note: related PR :